Add iam auth policy validation and handle targetRef changes #478
Conversation
controllers/errors.go
Outdated
| import "errors" | ||
|
|
||
| var ( | ||
| TargetGroupNameErr = errors.New("wrong group name") |
There was a problem hiding this comment.
nit: Error message is generic among all groups, so why is the error variable specific to target groups?
controllers/errors.go
Outdated
| var ( | ||
| TargetGroupNameErr = errors.New("wrong group name") | ||
| TargetKindErr = errors.New("target kind error") | ||
| TargetRefNotExists = errors.New("targetRef does not exists") |
There was a problem hiding this comment.
will change names
| IAMAuthPolicyAnnotation = "iam-auth-policy" | ||
| IAMAuthPolicyAnnotationResId = k8s.AnnotationPrefix + IAMAuthPolicyAnnotation + "-resource-id" | ||
| IAMAuthPolicyAnnotationType = k8s.AnnotationPrefix + IAMAuthPolicyAnnotation + "-resource-type" | ||
| IAMAuthPolicyFinalizer = k8s.AnnotationPrefix + IAMAuthPolicyAnnotation |
There was a problem hiding this comment.
Is this the typical finalizer format for CRD finalizers? We follow a different pattern in other controllers, like service.k8s.aws/resources and accesslogpolicy.k8s.aws/resources
There was a problem hiding this comment.
From what I've read, finalizers are typically in the format of example.com/finalizer. This appears to be something we should change across all our resources, and have one finalizer for any resource type (which makes sense, since we have no reason to put the resource Kind in the finalizer for that kind, the purpose of the finalizer name is to avoid collisions with other controllers).
This is a non-blocker for the current PR, I'll make an issue to address it.
| controllerutil.RemoveFinalizer(k8sPolicy, authPolicyFinalizer) | ||
| func (c *IAMAuthPolicyController) reconcileDelete(ctx context.Context, k8sPolicy *anv1alpha1.IAMAuthPolicy) (ctrl.Result, error) { | ||
| err := c.validateSpec(ctx, k8sPolicy) | ||
| if err == nil { |
There was a problem hiding this comment.
ignore validation errors on deletion
xWink
left a comment
xWink
left a comment
There was a problem hiding this comment.
Minor comments, can be addressed later. Liking the new utility functions, will be nice QoL improvements for other reconcilers.
| return model.IAMAuthPolicyStatus{}, err | ||
| } | ||
| resourceId := *svc.Id | ||
| err = m.enableSvcIAMAuth(ctx, resourceId) |
There was a problem hiding this comment.
If I understand it correctly from our previous standup meeting, for this issue: #467
When create the AuthPolicy, the order should be: (not correct in current PR)
- PutAuthPolicy
- Change AuthType to AWS_IAM
When create the AuthPolicy, the order should be: (correct in current PR)
- Change AuthType to NONE
- DeleteAuthPolicy
Do you need to change creation handling order?
There was a problem hiding this comment.
to put policy we first enable iam and then put policy, for delete we disable iam and remove policy
3 participants
Note:
Add spec validations
Add resource cleanup - delete previous policy - when spec is changed.
All these use-cases tested manually.
Code changes:
Leftovers: